Reviewed 4 of 9 files at r1, all commit messages.
Reviewable status: 4 of 9 files reviewed, 3 unresolved discussions (waiting on @Pururun)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/receiver/BootCompletedReceiver.kt line 12 at r1 (raw file):

class BootCompletedReceiver : BroadcastReceiver() {
    override fun onReceive(context: Context?, intent: Intent?) {
        if (intent?.action == "android.intent.action.BOOT_COMPLETED") {

Use Intent.ACTION_BOOT_COMPLETED

Code quote:

"android.intent.action.BOOT_COMPLETED"

android/lib/resource/src/main/res/values/strings.xml line 388 at r1 (raw file):

    <string name="failed_to_set_current_test_error">Failed to set to current - API not reachable</string>
    <string name="failed_to_set_current_unknown_error">Failed to set to current - Unknown reason</string>
    <string name="connect_on_start">Connect on start</string>

Reminder that we should align this string

Code quote:

Connect on start

android/lib/resource/src/main/res/values/strings.xml line 389 at r1 (raw file):

    <string name="failed_to_set_current_unknown_error">Failed to set to current - Unknown reason</string>
    <string name="connect_on_start">Connect on start</string>
    <string name="connect_on_start_footer">Automatically connect on device start-up</string>

Should we somehow clarify in this text that it only works if the permission has been approved? Or do we have some other idea on how to handle missing permission? 🤔

(this comment is also related to the VpnService.prepare(context) check)

Code quote:

Automatically connect on device start-up

Reviewable status: 4 of 9 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/receiver/BootCompletedReceiver.kt line 12 at r1 (raw file):

Done.

Reviewed 1 of 3 files at r2, all commit messages.
Reviewable status: 5 of 9 files reviewed, 3 unresolved discussions (waiting on @Pururun)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/receiver/BootCompletedReceiver.kt line 19 at r2 (raw file):

    private fun startAndConnectTunnel(context: Context) {
        // Check for vpn permission
        if (VpnService.prepare(context) == null) {

nit: I suggest a small helper function hasVpnPermission or val over the need for this comment

Code quote:

        // Check for vpn permission
        if (VpnService.prepare(context) == null) {

Reviewable status: 4 of 12 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/receiver/BootCompletedReceiver.kt line 19 at r2 (raw file):

Done

Reviewed 1 of 9 files at r1, 2 of 4 files at r3, all commit messages.
Reviewable status: 7 of 12 files reviewed, 3 unresolved discussions (waiting on @Pururun)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/receiver/BootCompletedReceiver.kt line 19 at r2 (raw file):

Looks good! 👍

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/ConnectOnStartRepository.kt line 15 at r3 (raw file):

    private val bootCompletedComponentName: ComponentName
) {
    val connectOnStart = MutableStateFlow(isConnectOnStart())

I think we should be a bit more clear and verbose in how we name/refer to this setting. Imo something like autoStartAndConnectOnBoot better describe what it does. One example where the current naming can be confusing is "start" as in "ConnectOnStartRepository" which could mean either on app, service or device start.

This comment applies to the whole PR.

Code quote:

connectOnStart

Reviewable status: 3 of 12 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/ConnectOnStartRepository.kt line 15 at r3 (raw file):

Changed it to what was suggested. I guess we can change it again if the text is changed to something wildly different.

Reviewed 4 of 9 files at r4, 9 of 9 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @albin-mullvad and @Pururun)

android/app/src/main/AndroidManifest.xml line 111 at r5 (raw file):

        <receiver android:name="net.mullvad.mullvadvpn.receiver.BootCompletedReceiver"
                          android:enabled="false"
                          android:exported="true">

Does it have to be exported? It means others' can invoke our receiver is it needed for when the system calls as well? Or does the intent filter protect us?

android/lib/resource/src/main/res/values/strings.xml line 388 at r1 (raw file):

Should these strings be updated according to what @albin-mullvad said?

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 15 at r5 (raw file):

    private val bootCompletedComponentName: ComponentName
) {
    val autoStartAndConnectOnBoot = MutableStateFlow(isAutoStartAndConnectOnBoot())

Do we know that this property, from anyone else than us? E.g adb or something like that.

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 36 at r5 (raw file):

            COMPONENT_ENABLED_STATE_ENABLED -> true
            COMPONENT_ENABLED_STATE_DISABLED -> false
            else -> error("Unknown component enabled setting")

Let's add all states instead of using else, if Google decides to add another one in a later stage:

        PackageManager.COMPONENT_ENABLED_STATE_ENABLED
        PackageManager.COMPONENT_ENABLED_STATE_DEFAULT
        PackageManager.COMPONENT_ENABLED_STATE_DISABLED
        PackageManager.COMPONENT_ENABLED_STATE_DISABLED_UNTIL_USED // Can this happen? When would it happen?
        PackageManager.COMPONENT_ENABLED_STATE_DISABLED_USER // Is there a case were we should care about this one?

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 40 at r5 (raw file):

    companion object {
        private const val BOOT_COMPLETED_DEFAULT_STATE = false

Annoying that we have to define this, it should be implicit from the manifest. Not sure if it is possible to grab some how?

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @albin-mullvad, @Pururun, and @Rawa)

android/app/src/main/AndroidManifest.xml line 111 at r5 (raw file):

I assumed it is because of the system sending the broadcast, but I can test without it.

android/lib/resource/src/main/res/values/strings.xml line 388 at r1 (raw file):

The strings have been aligned with Matilda, do we need to align it with someone else?

android/lib/resource/src/main/res/values/strings.xml line 389 at r1 (raw file):

String updated.

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 15 at r5 (raw file):

Not sure I follow, are you asking if this could be spoofed somehow?

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 36 at r5 (raw file):

Will do.

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 40 at r5 (raw file):

As far as I could tell no, but I can look into to it further.

Reviewable status: 13 of 14 files reviewed, 6 unresolved discussions (waiting on @albin-mullvad and @Rawa)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 36 at r5 (raw file):

Added all possible cases, still kept else since we are checking an int.
Note that PackageManager.COMPONENT_ENABLED_STATE_DISABLED_UNTIL_USED and PackageManager.COMPONENT_ENABLED_STATE_DISABLED_USER are only valid for applications and not components (even though name implies that they are..) but I guess it is useful if the behaviour of android changes.

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 40 at r5 (raw file):

So the setting works that it checks what was last set by setComponentEnabledSetting. If this has never been called, it will return "default" (sort of like null if a shared pref has never been set). So the only way I can think of is to refer to a common resource in the repository and in the manifest. Not sure how useful that is.

Reviewable status: 13 of 14 files reviewed, 4 unresolved discussions (waiting on @albin-mullvad and @Pururun)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 36 at r5 (raw file):

Discussed, should work with when.

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 40 at r5 (raw file):

Sounds good to me

Reviewable status: 13 of 14 files reviewed, 3 unresolved discussions (waiting on @Rawa)

android/lib/resource/src/main/res/values/strings.xml line 389 at r1 (raw file):

Text alignment issue added:
https://linear.app/mullvad/issue/DROID-1200/align-on-text-for-connect-on-boot-setting-on-tv

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 15 at r5 (raw file):

So it is possible to change this state via adb, will add a linear issue.
https://linear.app/mullvad/issue/DROID-1201/handle-external-enabledisable-of-broadcast-receiver

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 36 at r5 (raw file):

Does not work with when :(

Reviewable status: 12 of 14 files reviewed, 3 unresolved discussions (waiting on @Rawa)

android/app/src/main/AndroidManifest.xml line 111 at r5 (raw file):

Seems to work fine even though it is not exported.

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

android/app/src/main/AndroidManifest.xml line 111 at r5 (raw file):

I believe we can remove exported="false" it should be default, https://developer.android.com/guide/topics/manifest/receiver-element#exported

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 15 at r5 (raw file):

👍

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/AutoStartAndConnectOnBootRepository.kt line 36 at r5 (raw file):

My bad :)

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved